Skip to content

Comments

Split API classes and interfaces#2

Merged
jmikola merged 5 commits intomasterfrom
split-classes
Jun 16, 2014
Merged

Split API classes and interfaces#2
jmikola merged 5 commits intomasterfrom
split-classes

Conversation

@jmikola
Copy link
Contributor

@jmikola jmikola commented Jun 12, 2014

Split previous API files into many more classes and interfaces. Manager and Server APIs were left as-is.

It may be helpful to create a class hierarchy/diagram.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These factory methods may not even be necessary, as the userland layer can simply interpret the command response and construct the cursors outright.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, don't add this if it's not necessary... but, alternatively the constructor could just accept Server and CommandResult instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in video chat, the constructor cannot take a CommandResult directly, since that leaves us with now way to create command cursors from parallelCollectionScan result documents. We agreed to remove this factory method completely.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bjori: What was the intention of the database and connection options here? If we have Server instances, aren't the connections already established?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed that both arguments should be removed, and we're be swapping the constructor with fromDsn(), since DSN-based construction is the default convention. Servers can be manually constructed with their own options, and we'll have a general factory method to create a Manager with a specific collection of servers.

jmikola added 2 commits June 13, 2014 03:52
Some highlights: fix class/interface syntax; additional class/method docs; Manager convenience methods; more value objects.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and Query do not have getter methods, since I assume their data will be accessed internally. Users shouldn't have any reason to read these objects after creating them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went back to using an array in lieu of WriteOptions class, since the set of options is really not a value object. The write options array for the three single-operation convenience methods is comparable to the options used in constructing the WriteBatch classes, except the WriteBatch classes would require an ordered option as well (akin to what we currently have in MongoWriteBatch).

jmikola added a commit that referenced this pull request Jun 16, 2014
Split API classes and interfaces
@jmikola jmikola merged commit 74a9aac into master Jun 16, 2014
@jmikola jmikola deleted the split-classes branch June 16, 2014 15:16
derickr added a commit that referenced this pull request Nov 20, 2018
derickr added a commit that referenced this pull request Dec 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants